Release: staging → main (194 commits) - #420
Open
mindsdb-release-train[bot] wants to merge 194 commits into
Open
Conversation
…skills The 15.3k-char BACKEND_GENERATION_PROMPT and 10.7k-char VISUALIZATIONS_HTML_OUTPUT_FORMAT_PROMPT were re-sent in the system prompt on every LLM call. They now ship as read-only built-in skills (build-fullstack-backend, build-html-dashboard) served by SkillStore and are recalled on demand; the always-sent prompt carries short mandatory recall hints instead. create_artifact/launch_backend descriptions reinforce the recall. System prompt drops from ~43.5k to ~22.6k chars. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…e source of truth for HTML contract - load() falls back to the built-in when a same-label user dir exists but is unreadable (broken shadow no longer dead-ends a mandatory contract), and logs when a user skill shadows a built-in label. - recall_skill embeds a stable marker in its payload; repeat recalls return a short stub while the body is still visible in history, and re-send the full procedure if compaction evicted it. - build-fullstack-backend step 5 no longer references the deleted VISUALIZATIONS prompt section — it recalls build-html-dashboard, the single source of truth for dashboard HTML (inline defaults only as fallback). - Hygiene: revert unrelated uv.lock resync; provenance comment/docstring/ developer docs mention built-ins; list_all/list_summaries share one _iter_skill_dirs walk. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The idempotence stub embedded the same marker _already_in_history matches on, so a stub surviving compaction (while the full body was evicted) suppressed re-sends forever. Detection now requires the marker AND the procedure header in the same message (only the full payload has both, ensure_ascii=False so the em-dash header actually matches), and the stub carries neither. Regression tests: surviving stub and marker-quoting summary both trigger a full re-send. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… publish_or_preview
…e scratchpad (ENG-824) On a host whose default code page isn't UTF-8 (GBK/cp936 on Chinese Windows, and likely other CJK locales) LocalScratchpadRuntime crashed before any cell ran: `_BOOT_SCRIPT_PATH.read_text()` decoded the boot script (which contains `…`/`—`) with the locale default → `'gbk' codec can't decode byte 0xa6`. Fix, at every parent↔child byte boundary + interpreter-level: - Read/write the boot script as UTF-8 (read_text/encode). - Force UTF-8 mode in the subprocess env (PYTHONUTF8=1 / PYTHONIOENCODING=utf-8, via _utf8_env, setdefault so an explicit override wins) — so the child's file I/O and stdio are UTF-8 regardless of host locale. - Decode/encode the cell payload + stdout/install output as UTF-8 (errors="replace" on display output so odd bytes never crash the reader). Non-breaking: this content is already UTF-8 on the wire, so UTF-8-default hosts (macOS/Linux/English Windows) are unchanged; it only turns the hard crash into working on non-UTF-8-locale hosts. Tests: _utf8_env forces UTF-8 / respects overrides; the boot script must be read as UTF-8 (its bytes are not GBK-decodable). 96 existing scratchpad tests still pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…view, ENG-824) Self-review: a bare PYTHONIOENCODING=utf-8 downgrades the child's stdio error handler from surrogateescape → strict (verified), which adds nothing over PYTHONUTF8=1 (already utf-8 for open()/filesystem/stdio) and re-introduces a strict-encode crash on exotic output. Keep only PYTHONUTF8=1. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…elf-review, ENG-824) Skill review: the main scratchpad subprocess got _utf8_env but the dependency install subprocess didn't, so on a non-UTF-8 host locale pip/uv output could come back as mojibake. Pass env=_utf8_env(os.environ) here too for consistency. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ta (folder-aware) [ENG-844]
ENG-847: Fix scratchpad web_search() on the minds-cloud gateway
…pt read (PR #253 review, ENG-824) Address Zoran's review on #253: - _setup_parent_site_packages wrote _parent_venv.pth with a plain open() (host- locale encoded) while the child reads .pth as UTF-8 under UTF-8 mode — same class of bug as the boot script. Write it as encoding="utf-8". - Extract _read_boot_script() and add a test that spies on Path.read_text to assert the boot-script read passes encoding="utf-8" — the previous bytes-only test would still pass on UTF-8 CI if the explicit encoding were dropped. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…w, ENG-824) Self-review of the review-response commit: I pinned the boot-script *read* as UTF-8 but left the sibling .pth *write* fix untested — asymmetric with the exact concern Zoran raised. Add a regression test that spies on open() and asserts the _parent_venv.pth write passes encoding="utf-8" (bypasses the heavy __init__; verified it fails if the encoding is dropped). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…d-utf8 ENG-824: force UTF-8 in the scratchpad so non-UTF-8 host locales (GBK/CJK Windows) don't crash
feat(prompts): move backend + HTML-dashboard contracts into built-in skills
feat(publish): add access modes (password/restricted) to /publish and…
…ent errors (ENG-673) (#246) * fix(llm): back off + retry mid-stream provider failures; typed transient errors (ENG-673) A mid-stream overload arrives inside an HTTP-200 stream (the SDK raises APIStatusError with status_code=200, real reason in .body), so anton's status-only classifier surfaced the nonsensical "Server returned 200 — the LLM endpoint may be temporarily unavailable" and the session loop retried it instantly with zero backoff — burning all attempts within seconds of a minutes-long incident (BUG-CM-001, Anthropic incident 2026-07-08). - New `TransientProviderError` / `ProviderOverloadedError` + a shared `classify_transient` in provider.py. Classify by BODY, not status: overloaded/api_error, 5xx, plain-429, connection drops, truncated streams. - anthropic.py: refactor the two byte-identical status-only blocks into a shared `_raise_for_status_error` mirroring the ENG-598 openai mapper; openai.py: extend that mapper with the transient branch (covers all four paths). - session.py: budget-bounded backoff-and-retry (30s/turn, cancellation-aware, jittered ~2/10/18s) for the mid-stream case that had NO prior retry; on exhaustion raise ProviderOverloadedError (carries model+provider) for the cowork-server/cowork `provider_overloaded` card. Completed tool_results are never re-executed on retry (idempotency) — only dangling tool_use is sealed. - Split by prior-retry: request-time 5xx/429/connection errors (already SDK-retried) and truncated streams carry session_backoff=False — honest typed message, but fail fast instead of stacking another 30s. - Log the (scrubbed, via ENG-583 scrub_credentials) error body on every transient occurrence. Tests: tests/test_transient_retry.py (classifier, both mappers, backoff helpers, turn-level recovery / budget-exhaustion / cancel / no-replay). Updated the two ENG-598 mapper tests (429/500 now typed-transient) and the two e2e error-handling tests (honest message, fast fail). Full suite green except the 2 pre-existing environmental scratchpad-subprocess failures. Part 1 of 3 for ENG-673 (cowork-server + cowork companions to follow). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(llm): address adversarial-review findings on the transient-retry path (ENG-673) Self-review of the 3-PR stack surfaced four issues; fixing them here (anton side): - #1 (was a real regression): truncation detection raised whenever a stream ended with no finish_reason/stop_reason — but many OpenAI-compatible endpoints simply don't report one, so a COMPLETE, good answer was being discarded and turned into an error (and would fail every turn for such a provider). Now only the truly-empty case (no content AND no tool_calls) is treated as truncated; a content-bearing stream without a terminal marker is logged and passed through. - #3: user-stop DURING backoff re-raised the TransientProviderError, surfacing a provider-error card instead of a clean cancellation. Now it breaks cleanly (like a normal stop). - #4: ProviderOverloadedError always named the planning model even when the CODING model was the one that failed. TransientProviderError now carries the in-flight `model` (threaded through classify_transient + both providers' raise sites); the card names the actual failing model, falling back to planning. Tests updated for the new clean-cancel semantics + 2 new (model propagation, failing-model-not-planning). Full suite green (bar the 2 pre-existing environmental scratchpad failures). (#2 — an overstated "graceful degradation" claim — corrected in the PR #246 description, no code change.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(llm): regression guards for the truncation fix — content-without-finish_reason passes through, empty stream truncates (ENG-673) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(session): don't tell the model to "adjust your approach" on a provider blip (ENG-673 #6) A request-time TransientProviderError (5xx / rate-limit / dropped connection) reaching the count-based retry path was injected as "An error interrupted execution… adjust your approach to avoid the same error" — but that's a service hiccup, not the model's fault, so the note misattributes the failure and can degrade the next attempt mid-incident. Transient errors now get a neutral note ("a transient service issue, not a problem with your approach — continue as planned"); genuine errors keep the original recovery guidance. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(llm): recover mid-stream connection/APIError failures; keep empty-truncated fail-fast (ENG-673) Review-round fixes (Sam/SailingSF, anton#246): - session_backoff now means "did the SDK already retry?" — mid-stream failures (connection drop after the 200, read timeout) back off within the budget; request-establishment failures still fail fast. Adds a stream_started flag on all three streaming paths (anthropic, openai chat.completions, Responses API). - Catch the bare openai.APIError a mid-stream SSE error raises (it is NOT an APIStatusError) so the OpenAI/MindsHub path classifies + backs off instead of leaking a generic error; classify_transient now reads both the Anthropic (nested) and OpenAI (unwrapped, top-level) body dialects. - Empty-from-start truncated stream stays fail-fast: a broken/misconfigured endpoint must not loop the 30s budget (product decision; carved out of the ticket's "truncated -> recover"). - Real-SDK mock harness (tests/test_transient_retry_e2e.py) over httpx.MockTransport; resolves the dangling test_transient_retry_e2e reference. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ENG-742: Count turns without tools-calls
…-independent UTF-8) (#263) * fix(scratchpad): read/write scratchpad + chat files as explicit UTF-8 (ENG-940) Completes ENG-824's Fix #2 ("suspenders") that the belt-only fix left undone. The scratchpad/chat-path reads relied entirely on PYTHONUTF8 being set by the launcher, so any path that misses it (bare CLI, provisioned/Docker cowork, OpenClaw) re-crashed on a GBK/CJK Windows host at `code = script_path.read_text()`. Add explicit encoding="utf-8" to every text read/write in that path so they're launcher-independent (belt AND suspenders, as ENG-824 specced): - chat.py: script read (the root-caused crash site), .env read + append, published/legacy/pub_file JSON read + write. - core/backends/local.py: .python_version + requirements.txt read + write. Tests (tests/test_scratchpad_utf8.py): a real non-ASCII fixture proves the explicit UTF-8 read round-trips while a host-locale (GBK) read crashes-or- corrupts (payload-independent); a regression guard fails if any `.read_text()` in anton/chat.py drops encoding=. Both independent of PYTHONUTF8. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(scratchpad): surrogatepass the cell-payload encode — encode-side sibling of ENG-824 (ENG-940) Follow-up from ENG-940's new evidence (users sabrina/eddie/janis): a non-ASCII Windows path (pt-BR "Área de Trabalho", emoji filename) is surrogate-escaped into lone surrogates (\udcXX) when decoded on a non-UTF-8 host. When that string reaches the strict UTF-8 encode of the cell payload sent to the subprocess, it raises "UnicodeEncodeError: surrogates not allowed" and kills the whole session — the encode-side sibling of ENG-824's decode crash. - local.py: extract _encode_cell_payload() using errors="surrogatepass" so the host-side encode can't crash when the host isn't in UTF-8 mode; the subprocess (always UTF-8 mode) decodes the payload fine. - The two chat.py JSON writes need NO change: json.dumps defaults to ensure_ascii=True, so surrogates become ASCII \uXXXX escapes and never reach the encoder (verified). - tests: pin the surrogate-safe encode (strict raises, surrogatepass round-trips) and a broadened accented-Latin + emoji case that must pass through unmangled. Verified the surrogate test fails if the helper reverts to strict. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(scratchpad): use surrogateescape (not surrogatepass) for the cell-payload encode (ENG-940 self-review) Adversarial review of de6d6fb caught a real bug in my own fix. surrogatepass does NOT round-trip through the subprocess: the subprocess always runs in UTF-8 mode, so its stdin decodes with surrogateescape — and surrogatepass emits the 3-byte CESU form that surrogateescape then re-mangles (\udc81 -> three surrogates), so the path would not survive intact. surrogateescape is correct on both counts: it's the inverse of the os.fsdecode that created these lone surrogates (U+DC80..U+DCFF), so it restores the original path bytes, and it matches the subprocess's surrogateescape stdin decode — the path arrives verbatim. Verified end-to-end. Also fixes the test, which previously asserted a false surrogatepass/surrogatepass symmetry; it now decodes with surrogateescape (what the subprocess actually does) and genuinely discriminates — it fails if the helper reverts to surrogatepass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ronment (#430) * Let a scratchpad subprocess receive an explicit workspace env overlay * Pass the workspace env overlay from the manager to new pads * Pass the workspace env overlay from ChatSessionConfig into the scratchpad manager * Make DS_* secret tracking and value lookups per-turn isolated * Add a regression test for concurrent-turn credential scrubbing isolation * Keep the connection-test credential scrubbed under per-turn isolation * Trim the connection-test docstring to the project's comment-length rule * Give the CLI and REPL scratchpad managers an explicit data vault Without a vault the manager derives no DS_* overlay, so a pad inherited the whole process env instead. A pad's DS_* now come from the vault, which also means a DS_* exported in the user's own shell no longer reaches a cell; the end-to-end scrubbing scenario seeds a vault connection instead of the subprocess env to match. * Expose a public setter for the per-turn DS_* value map The connection-test flow reached into another module's private ContextVar wrapper to record the values scrubbing needs. * Give an artifact backend its own DS_* instead of the inherited environ The backend subprocess copied the parent's whole environment, so it saw every connection any turn had injected, disabled ones included. It now receives exactly the datasources its artifact declared, resolved through the session vault, and the project .env reaches it under the same only-if-unset rule the scratchpad uses. * Apply the workspace env overlay only where the key is unset It was overriding unconditionally and after the DS_* strip, so a project .env could replace PATH or an API key in the pad and define a DS_* the vault never had. Restores what apply_env_to_process did before the overlay replaced it. * Make a turn's DS_* value map authoritative and log a failed lookup A key missing from the map no longer falls back to os.environ, where a concurrent turn may hold a different value under the same name: redacting with it would miss this turn's secret and substitute the other turn's. A value lookup that fails now says so, names only, instead of silently leaving that connection unscrubbable. * Stop restoring datasource credentials into the process environment restore_namespaced_env cleared every DS_* in the process and reinjected this caller's connections, so one turn wiped a concurrent turn's credentials and published its own. It now only rebuilds the turn's registries and value map; credentials reach a scratchpad through the pad's own env, derived from the same vault. Tests that asserted the process env now assert the env a pad actually receives. * Keep a connection test's credentials out of the process environment Both test paths wiped every DS_* in the process and wrote the credential under test into it, so a concurrent turn's cell could read it and lost its own. The pad already receives these values through ds_env_override, and scrubbing through the turn's value map. * Cover two concurrent turns on divergent vaults end to end Runs both turns' pads at once against a polluted process env and reads what each child process actually received: each sees its own vault's password, and a connection one turn has disabled stays absent there even while the other turn runs with it enabled. * Keep per-turn DS_* state visible to the turn that opened it Every tool call runs in its own task and asyncio.create_task copies the context, so reassigning the ContextVar meant a connect made mid-turn registered into a copy that was discarded: the turn then scrubbed against an empty registry and a password the user had just given the agent reached the model verbatim. The ContextVar now holds a mutable container, opened once per turn and mutated in place, which a child task's writes reach without letting a sibling turn's writes in. * Resolve a backend's datasources with the vault's own env builder _collect_datasource_secrets also emits the _-prefixed bookkeeping fields that env_for deliberately drops, so a Drive connection handed the backend its picked-file ids and titles alongside the token, and it fell back to the local anton vault when a host keeps its own elsewhere. Resolving per declared ref through env_for fixes both and matches what the scratchpad and the relaunch path already do. Also applies extra_env before the DS_* strip, so a project .env cannot override a vault credential, and drops four assertions that could not fail: a derived pad env only ever holds namespaced names. * Pin that the coarse unknown-DS_* net still reads the process env Only the labeled lookup is gated to the turn's map. An operator-exported credential is caught by the coarse net, and nothing said so. * Open the per-turn DS_* scope at the turn boundary Opening it lazily on first write meant a host that had opened nothing lost whatever a tool handler registered, because tool calls are dispatched in their own task: on the CLI, a password the user handed over mid-turn registered into a discarded copy and then reached the model, which staging scrubbed. turn() and turn_stream() now open the scope before any tool runs, seeded from the ambient DS_* so a reader sees no change. * Say when a backend's declared datasource is not in the vault env_for returns None for a connection that has been deleted, and that was swallowed into an empty dict, so the backend died on its first query with nothing in the log saying why. Also stops the session vault lookup raising where the sibling attribute is read defensively, and corrects two comments that claimed guarantees the code does not make. * Name a declared datasource that is missing from the vault env_for returns None for a connection deleted after the artifact declared it, and that was swallowed into an empty dict, so the backend died on its first query with nothing in the log. Also reads the session vault defensively, the way the sibling attribute already was. * Give the scoped set the discard() a set is expected to have A test that landed on staging while this branch was open calls discard() on the registry, which a plain set had and the scoped stand-in did not. * Rebuild the turn's DS_* scrub state from the session's own vault The pod builds its session in a run_in_executor worker so its heartbeat keeps firing, and ContextVar writes made in that thread never reach the turn's task. The turn therefore opened an empty scope and scrubbed against nothing, sending a printed OAuth token to the model verbatim where staging redacted it. The turn boundary now rebuilds from the vault attached to the session, so it no longer matters where the host built it. Also drops -prefixed bookkeeping at the backend caller rather than trusting each vault to: TurnKeyDataVault.env_for keeps them despite documenting the opposite. * Restore the previous scrub state when a rebuild fails The registries were emptied before anything replaced them, so a raise partway through left the turn scrubbing against nothing and a printed credential reached the model. An unreadable datasources.md was enough to trigger it. * Create the workspace .env with owner-only permissions * fix(workspace): write and read the vault as UTF-8, not the host locale AntonSettings loads .anton/.env with env_file_encoding="utf-8" (config/settings.py:32), but Workspace wrote and read that file, and anton.md, with bare read_text/write_text. Those use locale.getpreferredencoding(), which is cp1252 on a Western Windows install, so a non-ASCII secret was stored as bytes the settings loader cannot decode and every later AntonSettings() raised UnicodeDecodeError. A value outside the code page, such as a CJK string, raised UnicodeEncodeError on write and was never stored at all. The same two-line issue applied to the consent write in cli._ensure_terms_consent. (cherry picked from commit ba6cf94) * Rebuild a turn's scrub state per connection so one bad record cannot blind it * Re-seed the turn's DS_* map from the environment at every turn boundary --------- Co-authored-by: Mohammed Alkindi <alkndymhmd692@gmail.com>
* fix(data_vault): remove TurnKeyDataVault's dead base_url override
ENG-2128: TurnKeyDataVault accepted a keyword-only base_url override,
but its one real call site (cloud_turn/session.py) constructs
positionally and never bound it - a value cowork-server put in the
oauth block's own base_url field was dead on the wire. The module
comment on ANTON_CLOUD_AUTH_BASE_URL_ENV already states the intended
design ("never taken from the wire request"), which the now-removed
kwarg contradicted by existing at all.
Removed rather than wired up (the alternative the ticket lays out):
ANTON_CLOUD_AUTH_BASE_URL, set correctly and independently per
environment by scratchpad-controller, is already the working source -
nothing needs cowork-server to steer this per-request.
Added a regression test confirming a stray base_url key inside the
oauth dict has no effect, so the field can't silently start mattering
again if some other producer starts sending it.
Companion PR in cowork-server: removes the field from the oauth job
payload's producer side.
Linear: https://linear.app/mindsdb/issue/ENG-2128/the-oauth-blocks-base-url-is-dead-on-the-wire-anton-constructs
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* fix(tests): await rebuild_session in test_rebuilt_session_is_given_a_vault
Unrelated to ENG-2128, but was breaking this PR's CI: rebuild_session
became async at some point without this test being updated to match,
so the un-awaited call returned a coroutine that never ran, and
fake_session's captured["config"] was never set (KeyError: 'config').
Confirmed pre-existing on staging independent of this PR's changes.
The other patched helpers inside rebuild_session (refresh_knowledge,
build_runtime_context, get_runtime_factory) are all called
synchronously, not awaited, so their existing sync-lambda mocks stay
valid unchanged.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
---------
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
* Replace aiohttp with httpx2 * Rmv dependency * Re-generate lock
…431) * feat(analytics): stamp a per-attempt turn id so the tool->turn join is exact (ENG-2243) `turn_index` is a POSITION in the history, not a turn identifier: `_turn_count` is seeded by counting the user messages the session was handed, and cowork-server rebuilds the session every turn. A retried or cancelled attempt therefore arrives with the same history and stamps the same `turn_index`. Measured on prod 2026-08-28..09-01 (mock rows excluded): 14.5% of desktop turn keys carried more than one row (worst 16, spanning 34 hours), and 18.5% of `tool_completed` rows joined to more than one `turn_completed` row. Web is 0% and is the control — the pod runs one turn per process. Adds `TurnCost.attempt_id` (a `default_factory`, so no construction site can forget) and stamps it as `turn_attempt_id` on both events. `turn_index` is left alone: the Langfuse trace name and the artifact-store index are built from it, and cowork-server has its own unrelated `turn_index` vocabulary. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(analytics): cover the non-streaming turn() path (PR #431 self-review) `turn()` opens its own books at session.py:3632 and calls `_emit_turn_cost` separately from `turn_stream`. The `default_factory` makes it correct by construction, but "correct by construction" is exactly how one path ends up with a field the other lacks, so pin it: two runs on the same seeded history, asserting turn_index repeats and the attempt id does not. Mutation-verified: replacing the factory with a constant fails it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(analytics): pin per-TURN uniqueness, not per-session (adversarial review) Every other test in this file builds a fresh ChatSession per turn, which is what cowork-server does. The CLI does not — it keeps one session for the whole conversation. So an id that were merely per-SESSION satisfied the entire suite while leaving every CLI turn in a conversation sharing one id, which is the exact ambiguity this PR removes. Found by mutation, not by reading: handing every turn a stable per-session id passed all 22 tests. This one fails under that mutation and passes on the real code. Also asserts turn_index advances across the three turns, pinning that the two fields stay independent. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(analytics): widen the omitted-reason key set to ten (ENG-2247 rebase) Fifth edit of the ENG-2247 rebase, and the one no conflict measurement predicted. `git merge-tree` reported 4 conflict regions; there were 5 places to change, because `test_a_caller_that_omits_reason_degrades_to_unclassified` is a test ENG-2247 ADDED and ENG-2243 never touched. Git only conflicts where both sides edited the same lines, so an exhaustive key-set assertion living in the other branch's new test is invisible to it — it just fails after the rebase: Extra items in the left set: 'turn_attempt_id' Note left in the test for whoever widens this event next: grep the key set, do not trust the conflict count. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(analytics): 64 real bits, cover the late finalizer, correct four stale docs PR #431 review (tino097 at `c0cf6b9e`). Behaviour and diagnosis were confirmed sound; these are the six things that were not. **The one real gap — a missing test, `session.py:2856`.** `_emit_turn_cost` reads the books it was HANDED rather than `self._turn_cost`, and nothing tested it. `session.py:4643` is the abandoned-generator path, where a newer turn may already own the shared slot — exactly the case the stamped-at-open design exists for. `test_the_id_is_stamped_at_books_open_not_read_at_emit` never invokes the emit, so changing that read to live session state passed the whole module while stamping the live turn's id on the abandoned turn's row, reintroducing the #309 mis-attribution one layer down. `test_a_late_finalizer_reports_its_own_attempt_not_the_live_turns` pins it; mutation-verified against the reviewer's exact suggestion, `(self._turn_cost or tc).attempt_id`, which it is now the only test to kill. **`uuid.uuid4().hex[:16]` is 60 bits, not 64.** Hex position 12 is uuid4's version nibble, so it is the literal `4` in every id ever generated — measured 2000/2000. The comment's width claim was wrong and so was its comparison to the `aid` install fingerprint, whose primary path is `sha256(...).hexdigest()[:16]` (`analytics.py:205`); uuid4 is only `aid`'s fallback. Now `secrets.token_hex(8)`: 64 real bits, no UUID object to build. The existing test checked length and charset only, so it could not see a fixed nibble; `test_all_sixteen_characters_carry_entropy` asserts every position varies and names position 12 when it does not. **The attempt id now reaches the `turn_cost` log line.** Two attempts of one turn produced two indistinguishable log lines — the ambiguity this PR removes on the analytics side, left in place on the only local forensics surface. That line is what survives the collector allowlist (ENG-1355) and, per ENG-2193, it is the only channel a desktop customer has, so the new property had no fallback at all. Dropping the argument passed 41 tests before `test_the_structured_log_line_carries_the_attempt_too`. **Four documentation corrections.** - `test_tool_row_and_turn_row_agree_on_the_attempt_id`'s docstring named "reading live session state" as the REJECTED alternative. It is the mechanism in use (`session.py:3229`). The tool row is safe because its emit is synchronous within its own live turn and the owner nulls the books at close — not because it reads stamped books. Left as written, it read as a guarantee that was never implemented, and would have kept passing if the emit were ever moved off the synchronous path. - `_emit_tool_completed`'s docstring said "the two join keys, and nothing else". The payload is ten keys and three join keys after ENG-2247's merge plus this. That paragraph is the privacy-audit enumeration, so a short list is the wrong kind of stale. - The register said `turn_attempt_id` is "empty when the tool ran with no turn books open" — unreachable, since both emit sites are inside the tool loop. Replaced with the divergence that IS reachable: with books closed `turn_index` falls back to `_turn_count + 1` while this would be `""`, so the two keys would disagree about whether a parent exists. - `_posthog_body`'s no-`$insert_id` rationale pointed at a problem this PR solves — that no sound natural key exists. One now does. Not adding it (`TurnCost.emitted` already stops a double emit, one layer earlier and for every sink), but the standing reason is redundancy, not the absence of a key. Full suite: 3,007 passed, 31 skipped. Rebased onto `f3bfa6ac` (six commits, no conflicts) before applying any of this. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(analytics): reference symbols, not line numbers — all five had drifted Every `file.py:NNN` pointer in the files this PR touches was already wrong. Four were introduced by `fc70969c` itself: I quoted the reviewer's numbers, which were taken at `c0cf6b9e`, then the rebase onto `f3bfa6ac` and my own log-line comment shifted them again — within the same day. session.py:3229 -> `_emit_tool_completed` was at 3237 session.py:4643 -> `_turn_stream_inner`'s finally was at 4660 session.py:2856 -> `_emit_turn_cost`'s handed-books read was at 2854 session.py:3632 -> `turn()` was at 3846 (this one predates fc70969) analytics.py:205 -> pointed at an unrelated comment line `session.py` itself carries zero `file.py:NNN` references, so the house style was already symbol names and these were the outlier. All five now name the function and quote the expression, which cannot drift. Two things the audit turned up that a number swap would have hidden: **The function I cited does not exist.** I wrote `_aid()`; it is `get_installation_id()`. Caught by grepping every symbol the new comments name — the same class of error as the stale numbers, and invisible to a line-number fix. **`get_installation_id`'s docstring has the identical 60-bit defect.** Its Returns clause promises "64 bits of entropy" for both branches, but the no-MAC branch persists `uuid4().hex[:16]`, which is 60 for exactly the reason tino097 identified in `attempt_id`. Docstring corrected; the derivation is deliberately NOT changed, because that would give every already-fingerprinted Docker install a new `aid` and break the continuity of that identity. Also corrects my own reply on that thread: the sha256 branch is primary because it is taken whenever the machine has a real MAC, not because it comes first. No behaviour change — comments, docstrings and one test docstring only. Full suite: 3,007 passed, 31 skipped. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…nnot pass silently (#434) * test(verifier): pin the served model so an alias repoint cannot pass silently The verdict eval picks its two models by alias NAME. An alias is a catalog pointer, not a model: `mindshub_air` was repointed off Kimi K2.6 to `gpt-5.6-luna` around 2026-08-10, so both slots now hold the same behaviour and the gate has reported green while covering one population twice (ENG-1687). Adds an identity check rather than the behavioural probe the ticket proposed. `LLMResponse.model` is already on every response (ENG-1638), so `_check_served_model` costs no extra call, cannot flake, and catches repoints whose consequence nobody predicted — kimi's Moonshot -> Fireworks move changed no narration but flips its tool_choice failure mode. Behaviour was already guarded by `test_verdict`; identity was the missing half. Watched failing live against prod on today's `mindshub_air` before the pin was set to its current value, and each new unit test watched failing under a mutation of the logic it covers. The ticket's other steps are not done, deliberately: there is no narrating alias left to re-pin the second slot to. Re-measured 2026-09-03, 0 narration characters on eight aliases, identical with `_VERIFIER_NO_PREAMBLE` stripped, and prod's `verifier_failure = 'truncated'` is 0 over 30 days. The docstring now says the matrix covers two provider shapes, which is what is true. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(verifier): address three self-review findings on the served-model pin Adversarial self-review of #434 at e4f34cc. All three are in the new code. 1. The pin caught the repoint but not the state it CAUSED. ENG-1687 is "both slots ended up holding one model and the gate stayed green" — and the obvious way to clear the pin's red is to update the map to the new id, which lands right back there if that id is the other slot's model. Distinctness now has a test instead of holding by accident, covering both routes in: identical aliases, and distinct aliases pinned to one model. 2. Only the coding provider was shimmed. `from_settings` builds planning and coding as separate objects even when both roles name the same alias, so any future `generate_object` or `chat` call in this file would have been silently unpinned — the same quiet-gap class the pin exists to close. Both providers are wrapped now, deduplicated by identity, each closing over its own inner. 3. `response.model` is remote text reaching an exception message and GITHUB_STEP_SUMMARY, and a newline in it injects lines into a CI artifact — a bogus "✅" row for an alias never checked. Reuses `identity.sanitize_model_name`, which anton already applies to this exact field before it reaches a prompt, rather than the hand-rolled isinstance check. Fix 3 initially passed with the sanitiser removed, so its test was written until it failed for the right reason. Each fix is mutation-verified: pin-to- duplicate and same-alias both red their test, and the sanitiser's removal reds the injection test on both the recorded value and the raise. Verified live that the planning path is now counted (3 raw completes -> 3 guard invocations across 2 verdict calls and 1 `generate_object`) with distinct inner bindings. Full unit suite 2892 passed / 31 skipped; full live eval 13 passed in 84s. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…rase (#433) * fix(session): card a provider failure instead of telling the user to rephrase (ENG-1361) A provider failure at request time — a 5xx, an unreachable host, an unconfirmed 429 — took anton's count-based retry path, which had no terminal that could produce an error card. It asked the model to explain the outage (a call needing the very provider that was failing) and, when that call failed too, emitted: An unexpected error occurred: Could not reach the model provider — check your connection or try again in a moment.. Please try again or rephrase your request. Rephrasing cannot reach an unreachable provider. Users followed the advice anyway: the reported incident shows the user retyping the same request and then leaving. 127 turns across 55 users in 30 days, on the current builds, on both desktop and web. Four changes: 1. A transient that outlasts the COUNT budget now raises ProviderOverloadedError, exactly as the TIME budget already did — the same product event reported the same way. Raised before the summarize prompt is appended, so no orphan "task has failed N times" is left in history; keeps anton's typed message (ENG-673's "returned 500") and adds only the attempt count; preserves code="rate_limited" for an unconfirmed 429, since telling a rate-limited user the provider is "having an incident" is the ENG-1537 mis-report. 2. The summarize path's re-raise allowlist becomes membership of CURATED_PROVIDER_ERRORS, declared beside the exception classes. The allowlist shape exposed every new type by default and was missed repeatedly — the new parametrised test shows it was short FIVE members, not the two the ticket named. Forgetting now means an exception propagates (a generic card) rather than becoming prose that blames the user's wording. 3. The rephrase advice is gone from the generic fallback AND from the SYSTEM summarize prompt, which taught the model to give it too. 4. Telemetry: retry_terminal_reason, provider_failure_kind and provider_http_status. Without these, change 1 would make the request-time and mid-stream terminals byte-identical in analytics — `code` splits them but is a card contract that never reaches PostHog, and the retry count was a local variable reaching nothing at all. Security: reviewed. Every TransientProviderError message is anton-authored template text plus a validated integer status and a hard-coded provider label (audited all construction sites), so interpolating it into the card forwards no provider body to the client. Change 2 strictly REDUCES exposure: types that previously rendered `str(exc)` as assistant text now propagate and get the server's redacted stand-in instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(session): address self-review findings on the ENG-1361 terminal Four issues found reviewing this branch against a fresh base. 1. The attempt count was described two ways in adjacent branches of the same `if`: "Retried 3 times without success" vs "didn't clear after 3 attempts". Three attempts are made (one initial + two retries), so the first overstated the retries by one — in a change whose whole subject is copy that misleads. Both now say "after N attempts". 2. `_stamp_retry_terminal` books `provider_http_status` from ANY exception, so a non-transient terminal (an SDK BadRequestError exhausting the attempt budget) records status=400 with kind="". Verified by executing it. Keeping that behaviour — the status is the only signal those turns carry and suppressing it to keep the pair symmetrical would discard real data — but the two fields answer different questions and the docstring now says so, with a test pinning the asymmetry so nobody tidies it away. 3. The cancelled-path clearing of the three new fields had no test; a mutation removing it passed the entire suite. Now covered by a Stop landing after a terminal was stamped. 4. The conversion is scoped on exception TYPE, so it also sweeps in the `bad_response` codes (empty_response, truncated_stream) — which anton classifies as a STRONG broken-endpoint signal, and which therefore inherit a card whose primary action is Retry and a CLI prompt defaulting to `retry` rather than `setup`. Not a regression (the prose it replaced also said "try again in a moment") and genuinely ambiguous by the classifier's own wording, so the behaviour stands — but it was an implicit decision and is now an explicit, named deferral. This change ships `provider_failure_kind=bad_response`, which is what will size the population; routing it is ENG-2264. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(session): address review findings 2-5 and 7 on #433 Deep review of this branch found five things the first two commits got wrong. Verified each independently before acting. **5 (the worst).** The unconfirmed-429 branch is deleted. It fired ONLY when `session_backoff=False`, which for a 429 means `velocity_confirmed` was False — i.e. precisely the 429s anton could NOT confirm were velocity limits. `classify_transient`'s own docstring names that population and the exact failure: a daily quota in a dialect the string-exact billing guards miss "would otherwise spend the whole budget waiting out a daily quota that resets at midnight — then be told it is not a credits problem." The copy said "this isn't a credits problem" and "waiting a moment and continuing should work", with a Retry that cannot succeed — this ticket's own failure shape, relocated. It also bought nothing: an unconfirmed 429 carries retry_after=None, so the rate_limited card had no interval to time-gate with. The test asserted `"credits" in body`, pinning the defect; it now asserts the opposite. **4.** The wrap-up call now goes through `plan_stream_with_recovery`. It was the only plan_stream site without it, so a history too long to summarize died instead of compacting — and curating ContextOverflowError turned that into a card-less "An unexpected error occurred.", strictly less than the prose it replaced, on the one failure anton can fix itself. **2.** The comment claimed forgetting a type means it "PROPAGATES (a generic card — safe)". False twice: the runtime guard is still an allowlist, and the generic path replaces anton's message with a flat string rendered as a BUTTONLESS alert. Comment now states what is true, including that four members have no card on either transport. **2b/7.** The module-walk test covered only provider.py — a new class in openai.py left the suite green. Widened to every module that defines one. **7.** Three mutants of new code survived all 2963 tests: the SYSTEM prompt's rephrase wording (the half of fix 3 governing the path that SURVIVES this change), the count terminal's model attribution (the test was vacuous — both candidate sources held the same string, and cowork-server reads this back to decide `reconnectable`), and `classify_transient`'s status_code plumbing (the sole production writer of provider_http_status; every field test hand-built the exception). All three now covered, plus the rate_limit_wait_limit telemetry row. Also: dropped "(ENG-1361)" from inside the SYSTEM prompt — a live model prompt whose reply is streamed to the user, so a paraphrase could put an internal ticket id in a customer's chat. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test: drop SDK internals from the status-stamp test (CI red on #433) `test_a_non_transient_terminal_still_books_its_http_status` built a real `openai.BadRequestError`, which needs `httpx` to construct a Response. CI failed with `ModuleNotFoundError: No module named 'httpx'` while the same test passed locally. CAUSE: staging migrated `httpx` -> `httpx2` (#428, `54043551`) — `pyproject` now pins `httpx2>=2.7,<3` and `session.py` reads `import httpx2 as httpx`. CI builds the MERGE ref, where `httpx` no longer exists as a package; my worktree was based on pre-migration staging, where it still did. Nothing to do with test ordering. (An earlier version of this message blamed order-dependent flakiness and said the cause could not be identified. That was wrong, and it was findable — `git log` on staging names the migration. Corrected here rather than left in the history.) The fix stands either way, and the migration is the argument for it: the test should never have reached for a third-party constructor. `_stamp_retry_terminal` reads `.status_code` via getattr, so a real SDK exception bought nothing and coupled the test to a dependency that was being swapped underneath it. Now uses the same minimal `_FakeStatusError` stand-in `test_transient_retry.py` already defines for this exact reason. Verified on the rebased branch (post-migration staging): 3009 passed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(session): keep the session's web tools off the retry wrap-up call Review note on #433: routing the wrap-up through `plan_stream_with_recovery` (review finding 4) silently attached the session's native web tools, which the raw `plan_stream` call it replaced did not. The wrapper adds them unconditionally from `self._native_web_tools`, independent of the `tools` argument, so passing only `system=` still got them. Two reasons that is wrong on this specific call, neither of which applies to the agent-loop callers: * the prompt says "Stop retrying" and asks the model to summarize and explain — handing it a research tool contradicts the instruction; and * that prompt embeds the raw error (`Latest error: {exc}`), which can carry file paths and user content, so a provider-side search could take a query derived from it to a search backend. That is a new hop for error text, introduced by accident. `plan_stream_with_recovery` gains `allow_native_web_tools: bool = True`. The default keeps all five existing callers byte-identical — there the tools are the point — and only the wrap-up passes False. It needs that method for the COMPACTION, not for what rides along with it. Checked the second-order path too: on overflow the wrap-up calls `_summarize_history`, which uses `self._llm.summarize(...)` — a separate method that never touches the native_web_tools injection. So no path from this call reaches a provider-side search. Mutation-verified, one test each: reverting the opt-out, dropping the argument at the call site, flipping the default to False (which would silently strip the whole agent loop's tools), and reverting finding 4 back to raw plan_stream. Suite 3012 passed; e2e error-handling green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ers (#435) * feat(root-cause): map the six new handler sentinels to deliberate tiers ENG-2248 adds `reason=` literals to six anton handlers. Every one needs a row in `_SENTINEL_REASONS`, or `test_every_sentinel_reason_is_mapped` fails and — worse — the reason lands in `unclassified` and quietly depresses the wall counts. Each is resolved AWAY from `external_wall` unless the wall is unambiguous, per the rule in that test: missing_file TIER_SELF the agent chose the path and can list the directory (same shape as the existing artifact_not_found) not_an_image TIER_SELF its own argument; the message names image_too_large TIER_SELF the accepted extensions / the limit read_failed TIER_UNCLASSIFIED bare `except Exception` — covers a bmp_convert_failed TIER_UNCLASSIFIED permissions wall AND a corrupt file with one sentinel, so it stays out of every trip rung `store_unavailable` (TIER_WALL) is reused unchanged for the absent draft store — an unambiguous wall that was already trip-eligible before this PR. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(telemetry): declare ok/reason on six anton tool handlers (ENG-2248) 18 of 27 tools never declared a verdict, so 3,038 of 18,108 `tool_completed` rows in the last 30 days carry `ok="unknown"` (16.8%) and the ENG-1276 substring fallback decides the streak for all of them. Migrates the six highest-volume anton handlers — recall_skill (1,153 calls), open_artifact (279), list_artifacts (273), memorize (129), read_image (128), create_skill_draft (72). Together 2,034 of the 3,038 unknown rows. This is NOT a measurement-only change, and the ticket has been corrected to say so. `_apply_error_tracking` opens with `if ok is not None:` and uses the verdict DIRECTLY to drive `error_streak`, which fires the resilience nudge at 2 and the circuit breaker at 5. So the migration is deliberately tiered: tier 1 verified successes -> ok=True behaviourally free: the legacy matcher finds none of its five markers in a success body, so the streak reset either way tier 2 indisputable failures -> ok=False a REAL change, accepted: the call cannot succeed on retry, so it belongs in the streak tier 3 ambiguous outcomes -> left as recall_skill's three NO MATCH ok=None returns, open_artifact's "no artifact found", memorize's "encoding is disabled" — each with a comment saying why read_image is the tree's first multimodal verdict. It returns a list ONLY on success: a list carrying ok=False would reach two documented gaps in the list arms of both tool loops (the nudge/breaker text is never appended there, and `_record_root_cause` is never called), so the model would be retried silently past the breaker. An AST seam guard pins that shape shut tree-wide. tests/test_tool_verdict_migration.py (15 tests) states both directions explicitly rather than leaving them to be discovered in production: each success preserves the pre-migration streak outcome and the pre-migration result text, and the two-failure nudge and five-failure breaker are asserted as ACCEPTED consequences for the newly labelled failures. Seven mutations verified against it, each killed by the intended test. Not complete, and not claimed to be. Projected residual after this PR is ~1,004 rows (5.5%), owned by: cowork-server 438 (set_status, lookup_connector, request_credentials, report_success/failure, set_field_status, label_connection, request_extra_field — a separate ticket), anton-unmigrated 532 (scratchpad 216, web_fetch 95, select_path 81, connect_new_datasource 68, publish_or_preview 34, ask_user 38), unlocated 34. That is a floor: the deliberate tier-3 branches inside the six migrated handlers keep emitting unknown, and their share is not measurable from PostHog today because unknown rows carry no reason. The exact figure lands one release after this ships. Security: no new input reaches a shell, filesystem path, or log. The `reason` strings are fixed literals from a closed vocabulary, never interpolated from tool input; the result text the model sees is byte-for-byte what it was before. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(telemetry): add the seam guard, cover every NO MATCH exit, unshadow a class anton#435 review. Three findings, all confirmed in code before acting. **The seam guard ENG-2248 asked for — built inverted, because the literal version cannot exist.** The ticket wants a guard that "fails when a handler returns a bare value where a `ToolOutcome` is expected". Measured: 9 handlers in `tool_handlers.py` alone have 49 bare returns, and three of those handlers are MIGRATED ones whose bare returns are deliberate tier-3 exits. Such a guard fails on day one, on purpose-built code. So it is inverted into an inventory lock over `anton/core/tools/`, which gets the property the ticket actually wanted — the nineteenth handler arrives LOUDLY — without requiring the migration to be finished first. The first version of that guard was decorative and my own mutation caught it: it pinned only the verdict-DECLARING set, so a new handler with zero verdicts was absent from that set and sailed through. It now pins the full census too. Both directions mutation-verified: a new unverdicted handler fails on the census, and `list_artifacts` regressing to bare returns fails on the subset. **Every NO MATCH exit is now pinned, not one representative.** `recall_skill` has three, and the existing test only ever reached the first (an empty store). The reviewer's mutation — a verdict at the third, the fuzzy-match load race — survived the entire suite. `test_every_no_match_branch_is_unverdicted_not_just _the_empty_store` drives all three with fixture-drift assertions so a store that stops producing a near-miss fails loudly rather than silently exercising the wrong branch. Verified: a verdict at branch 1, 2 or 3 now fails, and 2 and 3 are caught by nothing else. **`missing_file` renamed to `path_not_found`.** `missing_file` is already a CLASS in `root_cause.py`, reached by a different path — `_WALL_TYPES` maps FileNotFoundError to it, `_STATUS_WALLS` maps 404 to it, it is the sole member of `_EXACT_ONLY_CLASSES`, and it gets path-identifier extraction. A sentinel KEY of the same name resolving to a DIFFERENT class (`unknown_resource`) is a collision a future editor reads straight past. Tier and class unchanged; only the key. Also detached this PR from the `tool_completed` measurement ticket. The body's word "closes" had Linear pull that ticket out of Done two seconds after this PR was created; the claim was wrong on its own terms (those rows are unjoinable for want of a `conversation_id`, not a verdict) and is now stated as a correction without the magic word. That ticket is back to Done. Full suite: 3,036 passed, 31 skipped. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Release: staging → main
194 commit(s) queued for the next production release.
Changes
Contributors
Review checklist